Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert as a component #1464

Merged
merged 5 commits into from
Sep 29, 2023
Merged

feat(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert as a component #1464

merged 5 commits into from
Sep 29, 2023

Conversation

Tlahey
Copy link
Contributor

@Tlahey Tlahey commented Sep 28, 2023 •
edited
Loading

Changes description

image

Svelte part :

  • Export the VtmnAlertItem component in order to be used without the VtmnAlert.
  • Title / description can be set with a slot
  • Change the <p> to <span> in order to be ISO with other frameworks

Svelte / React / Vue :

  • Add a new property ariaLabelCloseButton
  • Remove the id no really necessary
  • Set the timeout property to 0 in order to keep the alert visible

Also checked https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/examples/alertdialog/
The div with a role alert doesn't need to have a aria-labelledby /describeby
image

Context

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
  • Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
  • Check your code additions will fail neither code linting checks.
  • I have reviewed the submitted code.
  • I have tested on related showcases.
  • If it includes design changes, please ask for a review design-system-core-team-design GitHub team.

Does this introduce a breaking change?

  • Yes
  • No (checked on the DKT project, the id is not used)

Other information

@Tlahey Tlahey requested review from thibault-mahe and a team as code owners September 28, 2023 07:52
@Tlahey Tlahey changed the title feat(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert is used as a component feat(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert as a component Sep 28, 2023
@Tlahey Tlahey mentioned this pull request Sep 28, 2023
Closed
lauthieb
lauthieb reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@lauthieb lauthieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Tlahey, thanks for this PR.

Regarding potential breaking changes, did you check elsewhere than the DKT project but in all repositories of our private GitHub organization?

The only warning I imagine it's about that Alert is an overlay in our design system, (and it is in this "Overlays" category), and here we are allowing to use it also inside interfaces (not overlayed).

https://github.com/orgs/Decathlon/teams/design-system-core-team-design @Sabrinavigil @SimonLeclercq what's your opinion? Will this change also have an impact on the design and documentation of overlay semantics?

@thibault-mahe what's your opinion please regarding these changes?

Thank you very much 🙏

thibault-mahe
thibault-mahe previously approved these changes Sep 28, 2023
View reviewed changes
@thibault-mahe
Copy link
Contributor

hi! thanks for this PR @Tlahey and for pinging me @lauthieb . Everything is ok for me here, the use of the slot is nice and allows the component to be more flexible, without bringing any breaking change. The only mini breaking change is that the aria-label of the close button is no longer set to "Close toast" by default, which you can quickly change if you want @Tlahey.

Okay fo me otherwise.

@Tlahey
Copy link
Contributor Author

Tlahey commented Sep 29, 2023

Hello @thibault-mahe I've done the fix on the aria-label ;)

@Tlahey Tlahey merged commit 590dd2e into main Sep 29, 2023
4 checks passed
@Tlahey Tlahey deleted the feat/vtmnAlertItem branch September 29, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lauthieb lauthieb lauthieb left review comments

@thibault-mahe thibault-mahe thibault-mahe approved these changes

@SimonLeclercq SimonLeclercq Awaiting requested review from SimonLeclercq

@Sabrinavigil Sabrinavigil Awaiting requested review from Sabrinavigil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tlahey @thibault-mahe @lauthieb